Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch references #1721

Merged
merged 11 commits into from
Jun 17, 2021
Merged

switch references #1721

merged 11 commits into from
Jun 17, 2021

Conversation

butonic
Copy link
Contributor

@butonic butonic commented May 20, 2021

A preview of the changes that need to be done when merging cs3org/cs3apis#130

currently uses a go.mod replace to use the generated go-cs3libs at butonic/go-cs3apis@2093bac

Initial notes

  • After merging this GetPath becomes obsolete because it was only used to merge a relative path with an id based reference that is stored in the share manager. With the new Reference type we can represent the combination with a single struct. Deferred to a subsequent PR.
  • When creating a stat response or listing the path needs to take into account the original reference: if it was an absolute path we need to return an absolute path. If it was a reference with a storage and node id we need to keep them, and just adjust the path.
    • The ResourceInfo needs to hold both, a Reference that includes the name AND an Id property that holds a Reference for the actual file id (composed of storage id and node id) of the node
  • The storage provider must not know its mount point. That is the job of the registry/gateway. We adressed that in https://github.com/cs3org/reva/pull/1678/files#diff-198f1004a921b3627f7572a239452974429a7de6e4fa47f445c2ad35d2cd9026L1103 by resolving the resource using the storage.FS interface ... when really it should be done by the drivers, tracked in Separation of concerns: storage providers should not know about their mount path #578

@butonic butonic requested a review from labkode as a code owner May 20, 2021 22:36
@update-docs
Copy link

update-docs bot commented May 20, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request introduces 20 alerts when merging 78e1e4b into 3aba8fa - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@labkode
Copy link
Member

labkode commented May 21, 2021

  • It needs to be possible to obtain an absolute path from any reference

@butonic
Copy link
Contributor Author

butonic commented May 21, 2021

@labkode can you elaborate? The gateway is currently path based. The new reference would allow directly forwarding all requests if they have a storage_id set. The response for a stat or list container countains ResouceInfo elements, which should contain a path relative to the resource that was used to make the request.

That being said: when a request is made with an absolute path, all resulting references must (and will) be path based only again.

Is that what you mean?

@refs
Copy link
Member

refs commented May 21, 2021

Reminder to update go.mod removing this replace github.com/cs3org/go-cs3apis => github.com/refs/go-cs3apis v0.0.0-20210520134557-97cdc479815b

u, err := getUser(ctx)
if err != nil {
return "", errors.Wrap(err, "eos: no user in ctx")
}

// parts[0] = 868317, parts[1] = photos, ...
parts := strings.Split(id.OpaqueId, "/")
// FIXME @butonic REFERENCE ... umm ... 868317/photos? @ishank011 HELP?!?
parts := strings.Split(id.NodeId, "/")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@butonic pretty sure we can change this to fileID, err := strconv.ParseUint(id.NodeId, 10, 64). I can't remember any scenario where we append the path to the inode. Plus looking at the code, GetPathByID is never used anyway, so we should be good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but maybe not as part of this pr?

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 20 alerts when merging ccc2403 into 2327bce - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 20 alerts when merging 4af4a25 into 2327bce - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@labkode
Copy link
Member

labkode commented May 25, 2021

@butonic the share manager will persist a share like:
share with id 123 points to resourceID abc:456.

A call to GetPathById(abc:456) is needed to obtain the path of that share and present it to the user.

A user won't access a share by doing ls /myshares/123. That is not user friendly.
I hope is clear. Ping me on Gitter if is not for a quicker chat.

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 20 alerts when merging 8eac1b8 into 597e362 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 20 alerts when merging d18452f into 597e362 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@butonic
Copy link
Contributor Author

butonic commented May 25, 2021

@labkode right. I think the first goal should be to just replace the reference. No change in how the current path or id based references are used. /myshares/someid is not user friendly and AFAICT is nothing that this PR will touch or introduce.

The namespace that a user will see when using the CS3 api to list a constainer depends on the type of reference he uses. When he uses an absolute path all returned references will be absolute. When he uses a mixed reference (with id & path) the returned references will be relative to the id. But again, I think that is something that will become clearer when rebasing the spaces PR on this.

@lgtm-com
Copy link

lgtm-com bot commented May 25, 2021

This pull request introduces 20 alerts when merging 0d9cfe3 into b1d57b9 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 20 alerts when merging 8d48d98 into 3fd8259 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 20 alerts when merging 9dfa8ec into 3fd8259 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@refs
Copy link
Member

refs commented May 26, 2021

@labkode @ishank011 We've come full circle with a topic: Should ResourceInfo contain 2 different References (see here), and if so, what would be a more descriptive name that carries semmantical meaning?

Some extracts from @butonic and I over RocketChat providing more context:

[...] in a stat request, the id must always be a Reference with storage_id and node_id set, but the response also needs to carry the name of the file

[...] think of RecourceInfo.Ref as Parent (either as id or as path) + Name

[...] a ResouceInfo needs two References: one is the Id of the Resource, the other is a Path to the Resource which can be given either as an absolute path, or as a relative path to a storage space.

If we really need both Reference members, their names should carry more semantic meaning. We thought of renaming ResourceInfo.Ref to ResourceInfo.Path. WDYT? It is not a huge of a deal, but adding more semantic meaning to the API should be always a goal.

@butonic
Copy link
Contributor Author

butonic commented May 26, 2021

note that ResourceInfo.Path was renamed to ResourceInfo.Ref in https://github.com/cs3org/cs3apis/pull/126/files#diff-b91600c359c2e479b2dbc31ff8dd5bd52bfaa48dc70c33a0de121215ff0c030eR68

I think we should leave the name Path but change the type from string to Reference

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 20 alerts when merging 6805f8f into 3fd8259 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 20 alerts when merging 7e777ec into 3fd8259 - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 20 alerts when merging f756df0 into 1c0c24a - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@butonic butonic changed the title [WIP] switch references switch references May 27, 2021
@butonic
Copy link
Contributor Author

butonic commented May 27, 2021

@labkode @refs @ishank011 I think this can be merged (after review). But it needs to be updated with the newly generated go libs from the official cs3 api changes in cs3org/cs3apis#126. Let me know what you think.

@micbar
Copy link
Member

micbar commented May 31, 2021

@labkode @ishank011 can you review / merge?

@labkode
Copy link
Member

labkode commented Jun 2, 2021

@butonic @refs we need to differentiate between a resource ID and a Reference. A resource ID is unique mapping 1 to 1 to a resource. On the other hand, multiple reference can point to the same ResourceID. They are used for different purposes.

The ResourceInfo needs to contain a ResourceID and Path as before (no change here).
The Reference should only be used as a method to getting information.

From a ResourceInfo:ResourceID and ResourceInfo.Path you can always construct a Refence.

Please change the PR accordingly.

@butonic
Copy link
Contributor Author

butonic commented Jun 2, 2021

@butonic @refs we need to differentiate between a resource ID and a Reference. A resource ID is unique mapping 1 to 1 to a resource. On the other hand, multiple reference can point to the same ResourceID. They are used for different purposes.

✔️

The ResourceInfo needs to contain a ResourceID and Path as before (no change here).
The Reference should only be used as a method to getting information.

✔️

From a ResourceInfo:ResourceID and ResourceInfo.Path you can always construct a Refence.

Only when you know to which node the path is relative to. I wonder why we need a path at all, and not just the basename of the resource. When listing a resource the Reference used to make the determines the path to the resource, anyway. A client listing /path/to/resource already knows that all children are prefixed with that path, e.g. /path/to/resource/child. Why is a name property with child not enough? I know that the gateway currently always looks up the path for a reference. When the reference holds a spaceid we don't need to do that and can just forward the request to the correct storage provider. When listing the available spaces clients always will have references with a space id.

I used a Reference for Path, because there is a corner case for the spaces api: when making an id based request we need to know the parent id so that we can build the full path relative to the storage space root. So I used a Reference to return both, the Id of the parent and the name of the child.

In any case the same reasoning applies to all cases: path, id based and mixed requests. The returned path should be relative to the reference that was used to make the request. But that makes lookung up the path for an id impossible.

Please change the PR accordingly.

I will try to use a path and check with our spaces implementation. IIRC we need to be able to get to parent from an id only based reference.

@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2021

This pull request introduces 20 alerts when merging 7b5bce9 into 6ace58c - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

refs and others added 2 commits June 15, 2021 09:38
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2021

This pull request introduces 20 alerts when merging 91a9c7b into 07adb3f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2021

This pull request introduces 20 alerts when merging fb291f1 into 07adb3f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2021

This pull request introduces 20 alerts when merging bc66e39 into 07adb3f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2021

This pull request introduces 20 alerts when merging 9f231fe into 07adb3f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@labkode
Copy link
Member

labkode commented Jun 17, 2021

@butonic can you make LGTM happy as well? I think is important from a security perspective, specially when dealing with files.

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @butonic. Just a couple of minor comments

},
},
}
req := &provider.InitiateFileDownloadRequest{Ref: &provider.Reference{ResourceId: fileInfo.Id}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this as Path: fileInfo.Path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initiateDownload uses a ResourceInfo. The only stable identifier we get from that is the id. Maybe it should use a new Reference as the parameter?

func (action *DownloadAction) Download(fileInfo *storage.ResourceInfo) ([]byte, error) {

also should use a Reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm taking a look at:

func (action *DownloadAction) DownloadFile(path string) ([]byte, error) {
	// Get the ResourceInfo object of the specified path
	fileInfoAct := MustNewFileOperationsAction(action.session)
	info, err := fileInfoAct.Stat(path)
	if err != nil {
		return nil, fmt.Errorf("the path '%v' was not found: %v", path, err)
	}

	return action.Download(info)
}

shouldn't Download return a not found error? we should not make an extra stat call IMO. between the stat and the actual download someone else might have deleted the file...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really say, haven't looked at this code

pkg/storage/fs/owncloud/owncloud.go Outdated Show resolved Hide resolved
pkg/storage/fs/s3/s3.go Outdated Show resolved Hide resolved
@butonic
Copy link
Contributor Author

butonic commented Jun 17, 2021

@labkode the file traversal cases are in progress by @C0rby
@ishank011 me and @C0rby will address your issues asap!

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2021

This pull request introduces 20 alerts when merging eb98f1d into 692f38f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

pkg/utils/utils.go Outdated Show resolved Hide resolved
pkg/storage/fs/owncloud/owncloud.go Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2021

This pull request introduces 20 alerts when merging ea4cf92 into 692f38f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2021

This pull request introduces 20 alerts when merging b61df27 into 692f38f - view on LGTM.com

new alerts:

  • 20 for Uncontrolled data used in path expression

@ishank011 ishank011 merged commit 9740eed into cs3org:master Jun 17, 2021
butonic added a commit to butonic/reva that referenced this pull request Jun 17, 2021
author Jörn Friedrich Dreyer <jfd@butonic.de> 1623945204 +0200
committer Jörn Friedrich Dreyer <jfd@butonic.de> 1623958936 +0000

switch references (cs3org#1721)

Co-authored-by: A.Unger <zyxancf@gmail.com>
Co-authored-by: David Christofas <dchristofas@owncloud.com>

rebase

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix nil

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

update changelog

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add storage provider list spaces interface

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add stubs

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

initial ocis implementation for list storage spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

minor fixes

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add thoughts on proper spaces persistence layout

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Initial spaces implementation.

Signed-off-by: Klaas Freitag <kfreitag@owncloud.com>

more spaces work

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

filter spaces based on permissions, return name

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

resolve linter issues

implement storage space support into the storageprovider

Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix dav spaces href

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

GET preparations

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

WIP: spaces datatx

Add spaces.go

distinguish spaces/simple datatx

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make GET work for spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

MKCol implementation for spaces WIP

refactor CreateDir

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make MKCOL work for spaces

implement delete for the spaces api

fix: unwrap the requested reference before using it further

implement MOVE for spaces

simplify check if request body is empty

setup constants for webdav verbs

fix listcontainers for spaces references

implement PROPPATCH for spaces

implement COPY for spaces

add cases for lock, unlock and report for spaces

implement PUT for spaces

implement POST for spaces

implement HEAD for spaces

clean up and deduplicate webdav HEAD code

clean up and deduplicate webdav DELETE code

clean up and deduplicate webdav GET code

clean up and deduplicate webdav PROPFIND code

clean up and deduplicate webdav MKCOL code

clean up and deduplicate webdav MOVE code

clean up and deduplicate webdav PROPPATCH code

clean up and deduplicate webdav COPY code

clean up and deduplicate webdav TUS POST code

clean up and deduplicate webdav PUT code

try fixing tests

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix path checks before calling WalkPath

update the owncloudsql storage driver

fix spaceid handling

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix linter issues

fix CreateDir in the owncloud storage driver

fix non space path handling

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

allow creating empty files

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

no longer hardcode storageid in the driver

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

decomposedfs: create storage spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Id -> ID

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

move change-references changelog to unreleased

add spaces changelog

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add grant test for spaces

goimports

update to embedded reference

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

relative references should only return the basename

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

do not show shared resources as spaces for owners

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make sure the usage of user provides paths is secure

list spaces from all providers

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
butonic added a commit to butonic/reva that referenced this pull request Jun 17, 2021
author Jörn Friedrich Dreyer <jfd@butonic.de> 1623945204 +0200
committer Jörn Friedrich Dreyer <jfd@butonic.de> 1623958936 +0000

switch references (cs3org#1721)

Co-authored-by: A.Unger <zyxancf@gmail.com>
Co-authored-by: David Christofas <dchristofas@owncloud.com>

rebase

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix nil

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

update changelog

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add storage provider list spaces interface

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add stubs

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

initial ocis implementation for list storage spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

minor fixes

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add thoughts on proper spaces persistence layout

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Initial spaces implementation.

Signed-off-by: Klaas Freitag <kfreitag@owncloud.com>

more spaces work

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

filter spaces based on permissions, return name

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

resolve linter issues

implement storage space support into the storageprovider

Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix dav spaces href

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

GET preparations

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

WIP: spaces datatx

Add spaces.go

distinguish spaces/simple datatx

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make GET work for spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

MKCol implementation for spaces WIP

refactor CreateDir

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make MKCOL work for spaces

implement delete for the spaces api

fix: unwrap the requested reference before using it further

implement MOVE for spaces

simplify check if request body is empty

setup constants for webdav verbs

fix listcontainers for spaces references

implement PROPPATCH for spaces

implement COPY for spaces

add cases for lock, unlock and report for spaces

implement PUT for spaces

implement POST for spaces

implement HEAD for spaces

clean up and deduplicate webdav HEAD code

clean up and deduplicate webdav DELETE code

clean up and deduplicate webdav GET code

clean up and deduplicate webdav PROPFIND code

clean up and deduplicate webdav MKCOL code

clean up and deduplicate webdav MOVE code

clean up and deduplicate webdav PROPPATCH code

clean up and deduplicate webdav COPY code

clean up and deduplicate webdav TUS POST code

clean up and deduplicate webdav PUT code

try fixing tests

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix path checks before calling WalkPath

update the owncloudsql storage driver

fix spaceid handling

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix linter issues

fix CreateDir in the owncloud storage driver

fix non space path handling

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

allow creating empty files

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

no longer hardcode storageid in the driver

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

decomposedfs: create storage spaces

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Id -> ID

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

move change-references changelog to unreleased

add spaces changelog

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add grant test for spaces

goimports

update to embedded reference

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

relative references should only return the basename

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

do not show shared resources as spaces for owners

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

make sure the usage of user provides paths is secure

list spaces from all providers

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic deleted the new-reference branch July 8, 2021 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants